Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

docs: Fallback to reference description for responses #826

Merged
merged 4 commits into from
Oct 11, 2024
Merged

docs: Fallback to reference description for responses #826

merged 4 commits into from
Oct 11, 2024
+106 −2

Conversation

Nelspike
Copy link
Contributor

@Nelspike Nelspike commented Oct 4, 2024

Checklist

This PR attempts to resolve the issue on #795 by resolving the $ref given in the schema and fetching that description in case there's none. PTAL! 🚀

@Nelspike Nelspike mentioned this pull request Oct 4, 2024
Closed
2 tasks
jean-michelet
jean-michelet reviewed Oct 5, 2024
View reviewed changes
Copy link
Contributor

@jean-michelet jean-michelet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR!

I don't know the code base so I can't approve myself.

lib/spec/openapi/utils.js Outdated Show resolved Hide resolved
lib/spec/openapi/utils.js Outdated Show resolved Hide resolved
lib/spec/openapi/utils.js Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@jean-michelet
Copy link
Contributor

Does it solve your bug @melroy89?

@melroy89

This comment was marked as outdated.

Sign in to view
@Nelspike
Copy link
Contributor Author

Nelspike commented Oct 7, 2024 •
edited
Loading

First of all, @jean-michelet, thank you for the review! 🚀

Secondly and equally thankful to @melroy89 for the detailed reproduction description 💪🏼 From what I can see from your code, the only reason why this wouldn't work is due to the trailing # on your $ref, which is notFoundError#. That $ref is absolutely correct, so we need to find a way to resolve these properly in this PR. If you want to give it a try without the trailing #, I think that would do the trick, at least for this example!

@melroy89

This comment was marked as outdated.

Sign in to view
@Nelspike
Copy link
Contributor Author

Nelspike commented Oct 7, 2024

@melroy89 - That's exactly what I meant in my reply, we need to consider those use cases because your reference is 100% correct 😅 The code I pushed in the PR didn't take that into account yet, but just pushed a change that does! I never meant that you should remove #, but that the code I had previously worked without it.

@Nelspike
Copy link
Contributor Author

Nelspike commented Oct 7, 2024

Seems like the second use case you mentioned on your EDIT is blocked by this, @melroy89: #676

At the moment, seems like definitions is not supported within OpenAPI definitions on fastify-swagger, which difficults using refs like commonResponse#/definitions/notFound. However, your first use case with # at the end should be covered by this PR now 👍🏼 Once the other one's merged, it should be supported out of the box!

cc @asc11cat @Eomm @ivan-tymoshenko, tagging you folks for visibility since you were discussing on the other PR 😄

@melroy89
Copy link
Contributor

melroy89 commented Oct 7, 2024

However, your first use case with # at the end should be covered by this PR now 👍🏼 Once the other one's merged, it should be supported out of the box!

Let me test it again..

@melroy89
Copy link
Contributor

melroy89 commented Oct 7, 2024 •
edited
Loading

Yup works now! Thank you so much! I was looking for this for years.

@Nelspike I would like thank you by providing a donation! Any payment method will do.

@Nelspike
Copy link
Contributor Author

Nelspike commented Oct 8, 2024

@melroy89 - Thank you for your generosity and recognition! I'm totally okay doing this without a donation whatsoever, happy to help wherever I can 🎉

@melroy89
Copy link
Contributor

melroy89 commented Oct 8, 2024

@melroy89 - Thank you for your generosity and recognition! I'm totally okay doing this without a donation whatsoever, happy to help wherever I can 🎉

OK, if you change your mind. Let me know.

@melroy89
Copy link
Contributor

melroy89 commented Oct 9, 2024

Now hopefully this PR gets merged somewhere before 2030 :)

@jean-michelet
Copy link
Contributor

Now hopefully this PR gets merged somewhere before 2030 :)

I don't think this type of comment is going to help us move any faster. If @climba03003 is ok with the code, I will merge it.

@melroy89
Copy link
Contributor

melroy89 commented Oct 9, 2024

Now hopefully this PR gets merged somewhere before 2030 :)

I don't think this type of comment is going to help us move any faster. If @climba03003 is ok with the code, I will merge it.

No one can take a joke anymore.

gurgunday
gurgunday approved these changes Oct 9, 2024
View reviewed changes
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Haha don't worry @melroy89, I assure you we'll get this done this year :)

I just need another member (@climba03003 would be golden) to review

climba03003
climba03003 requested changes Oct 10, 2024
View reviewed changes
README.md Outdated
@@ -468,6 +468,9 @@ fastify.get('/responseDescription', {
}
}, () => {})
```

Additionally, if you provide a `$ref` in your response schema but no description, the reference's description will be used as a fallback.
Copy link
Member

@climba03003 climba03003 Oct 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please state the $ref currently only support the format of matching $id.
It cannot lookup the fragment part. For example, schemaId#/definitions/subSchema

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one, would pointing to the PR that's currently in the works (#676) suffice? 👍🏼

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in a82f2a7, @climba03003 - Feel free to take another look!

@gurgunday gurgunday requested a review from climba03003 October 10, 2024 15:49
climba03003
climba03003 reviewed Oct 11, 2024
View reviewed changes
README.md Outdated Show resolved Hide resolved
@gurgunday @climba03003
Update README.md
658ad1c 
Co-authored-by: KaKa <[email protected]>
Signed-off-by: Gürgün Dayıoğlu <[email protected]>
gurgunday
gurgunday approved these changes Oct 11, 2024
View reviewed changes
Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

I can release tonight, if anyone's free, feel free to merge and release

@jean-michelet jean-michelet merged commit ac99039 into fastify:master Oct 11, 2024
11 checks passed
@jean-michelet
Copy link
Contributor

Release tonight plz @gurgunday!

@mottet mottet mentioned this pull request Oct 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jean-michelet jean-michelet jean-michelet left review comments

@melroy89 melroy89 melroy89 left review comments

@gurgunday gurgunday gurgunday approved these changes

@climba03003 climba03003 climba03003 approved these changes

@Uzlopak Uzlopak Awaiting requested review from Uzlopak

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Nelspike @jean-michelet @melroy89 @climba03003 @gurgunday